-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Make SecretBuffer and shred! public #59724
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Wouldn't this be better in a package? To me this seems like a prime example of functionality that maybe shouldn't have been in Base |
Good point. We needed this in stdlib/LibGit2, so would you want to make this a new stdlib (./contrib/new-stdlib.sh) and move the getpass and SecretBuffer code there instead? |
I'll give it a go |
No way we make a whole new stdlib for this. |
A fair amount of packages already use it and https://juliahub.com/ui/Search?type=code&q=shred! 447 hits, so seems too late to back out of having it (despite it never an official API, is it known to be bad/ly designed or anything, or only that it should maybe never have lived in Base?) or move. Yes, a stdlib may be overkill unless it lives in stdlib/LibGit2 (similar to download should have not been in Base). Should it be deprecated in Base and/or moved? I don't care much, I just felt it might be usable as is, and public API warranted, even more now I see it used. Can it be moved still have forwarder wrapper in Base? Isn't it then as good to leave it there? Could do public AND deprecate. And postpone moving. I don't know yet how much code this is, any burden to have it in Base and/or sysimage? Should I add public for Base.getpass and then also for:
I didn't check yet if either or both used in packages, or if only the other two make sense. [I changed to draft just because I'm unsure about those other, that could be same or separate PR. Not sure how to add "DO NOT MERGE" yet label or if I can.] |
Hm yeah that's a lot of usage in the ecosystem. It was never documented in the manual, so removing it is fair game. That being said, I agree the polite thing would be to deprecate it first. Maybe have it be deprecated in 1.13, then remove it in 1.14? |
I don't really want to see this become public, personally. It's true the only reason this exists in the first place was to get LibGit2 working for v1.0 (#27565). It's a little kludgy, and there's no other reason it needs to be in base. I'd much rather see it split into an external package. |
(soft) deprecate right away in 1.12? But not remove it, even ever, just document a substitute package when we have it, at least if that happens or has better API or implementation, or even the same.
I think you're selling yourself short and the need for it in Base. People need to be able to write secure code, more than do math, e.g. have atan or have BLAS, and the rather small code is already there to help with that. Another reason to have anything in Julia is if used by Julia itself, and sort of true here; for LibGit2, you could say it shouldn't need to be bundled with Julia and be right, still the code here, shred! etc, could be bundled... What's the kludge (and is it in the implementation, at least the "API" itself ok?), since you wrote it... and it seems to need to be in Base, since a lot of packages depend on it? So does is have a snowball change of being split into a package? I also think we could make it public AND deprecate it...
Isn't that really needed (for anyone doing such correctly?), and while yes, could be a package, if would need to do the same, more or less all the functions. People can shred! regular strings (if careful, might do it incorrectly since UTF-8 is variable length), but it's very likely most users might not and would screw up the security, so at least the correct way needs to be very discoverable. I note (so why not make "best practice" API?):
[Because they are immutable? You mean otherwise you could shred! them?] Interesting, and I find it unlikely people would implement this right if on their own:
This is needed but needs not be part of the public API (not "used" by any package, or almost; only for/by precompile statement of IonCLI):
I doubt this should be made public (only used by BinaryBuilder tests):
|
I'm assuming can and should (sometimes) be used, not just by Julia. shred! ok in Strings? Maybe it or both should go under IO?
Co-authored-by: Andy Dienes <[email protected]>
18a4b87
to
2565039
Compare
Here's why: it does not have a well-defined security boundary or threat model. The binding that should be publicized instead, IMO, is |
I'm assuming can and should (sometimes) be used, not just by Julia.
[Was it ok where put the public declaration under: shred! ok under Strings? Maybe it or both should go under IO? Where how no effect, except sort of [mis]documenting, the main thing is if this should be made public at all.]